Use a local Walk function #421

d-a-v · 2019-09-22T21:03:00Z

Because arduino-cli segfaults when sketchDir is a symbolic link,
a simple walk function is introduced with a twofold effect:

fix the segfault
allow to use symlinks

ref: https://www.google.com/search?q=golang+Walk+does+not+follow+symbolic+links

edit:

Because arduino-cli segfaults when sketchDir is a symbolic link, a simple walk function is introduced with a twofold effect: - fix the segfault - allow to use symlinks ref: https://www.google.com/search?q=golang+Walk+does+not+follow+symbolic+links

CLAassistant · 2019-09-22T21:03:18Z

All committers have signed the CLA.

cmaglie · 2019-10-08T11:53:17Z

Hi @d-a-v, thank for your submission.

We already have an implementation of the "walk-folllowing-symlinks" here:
https://github.com/arduino/arduino-cli/blob/master/legacy/builder/gohasissues/go_has_issues.go#L39

BTW, I was wondering why the golang Walk function doesn't follow symlinks and, looking at the golang bugtracker, I found that is because it may lead to infinite loops:
golang/go#4759

This seems to be not golang-related but also other language that allows following symlinks suffer the same problem, for example in python we have:

https://docs.python.org/3/library/os.html#os.walk

that warns:

Note Be aware that setting followlinks to True can lead to infinite recursion if a link points to a parent directory of itself. walk() does not keep track of the directories it visited already.

Since it seems that there isn't a saner/simple way to avoid loop we want to propose the following solution:

Follow symlink only once on sketchPath
Once sketchPath is resolved, use plain golang Walk from there

this should cover the use cases in #424 (and I think 99.99% of the overall use cases).
What do you think?

d-a-v · 2019-10-09T15:00:18Z

Thanks,

I'm happy to close this PR when #424 is fixed by another mean.

(edited)

About the walking issue with symbolic links, we can add a decreasing counter in the recursive walk function (like the factorial recursive function we learn at school), and return an error (like ~~Too many levels of symbolic links~~ Too many levels of sub-directories) when it reaches 0.
That way there is no need for specific code anywhere, and we can have symbolic links for sketch dir, sketch files, utility/ and files under utility/.

I've also read the golang issues in their repository and I just don't understand why or how they would rewrite (or avoid) the unix features their own way. A symbolic link loop is on user responsibility Golang sys lib's responsibility is only to not fail (heap here), not to remove any OS functionality IMHO.

$ ls -l t
lrwxrwxrwx 1 user group 1 Oct  9 16:42 t -> t
$ cd t
bash: cd: t: Too many levels of symbolic links

rsora · 2019-10-15T12:38:05Z

Hi @d-a-v ,
May we ask you to add a couple of test for this PR in order to outline your walk function behavior?

We want to use your PR as a starting point to introduce a more robust walk function, following symlinks only once on sketchPath (as @cmaglie mentioned), in order to fix rapidly the bugs and avoid symlink related issues.

As a second step, we will make the walk generic, adding a symlink loop check mechanism like https://gitlab.openebs.ci/openebs/maya/blob/5bfd0e581446d188292000626cc4dd1150e4d368/vendor/golang.org/x/tools/internal/gopathwalk/walk.go#L172

Thanks for your help!

d-a-v · 2019-10-15T13:51:17Z

I'd be glad to.

First I need to know whether you agree with the idea that a path depth shall not be more than a given constant (like 40, similar to what's done in linux kernel "Linux imposes a limit of at most 40 symlinks in any one path lookup") ?

Shall I make this proposal over this file (instead of mine) ? Is it intended to stay where it is ?

masci · 2019-10-15T14:06:33Z

My 2c:

40 is good because it's what the Linux kernel uses
let's not add anything to the legacy package, I'd keep the function where it is now in sketch.go, we can move it later if other modules/packages need to walk the filesystem

rsora · 2019-10-15T14:41:33Z

@d-a-v go ahead using 40! Please keep the walk in sketch.go as @masci said. Thanks!

- act on error in walk function (OS can detect symbolic links loops (lnk -> lnk)) - add a deepness detector for non trivial loops (dir/lnk -> ../..)

d-a-v · 2019-10-23T21:34:11Z

Testing the error was missing in the user walk function,
Can you please validate the print I added there ?

A reference to this issue is added in a comment about the discussion on 40.
Usually one doesn't write raw constants.
Please direct me to the golang way/place for doing better if the comment is not sufficient.

d-a-v · 2019-10-24T08:58:59Z

How to test this code (with linux):
In some sketch directory, create a symlink loop, detectable by OS:

$ mkdir utility
$ cd utility
$ ls -s x x
$ arduino-builder ...
[...]
error: stat /home/gauchard/dev/proj/arduino/y/utility/x: too many levels of symbolic links
[...]

(OS error printed)

Or not detectable by OS:

$ mkdir utility
$ cd utility
$ ln -s .. x
$ arduino-builder ...
[...]
error: Filesystem bottom is too deep (directory recursion or filesystem really deep): /path/to/mysketchdir/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x
github.com/arduino/arduino-cli/arduino/builder.simpleLocalWalkRecursive
        /local/users/gauchard/arduino/gh-arduino-cli/arduino/builder/sketch.go:77
github.com/arduino/arduino-cli/arduino/builder.simpleLocalWalkRecursive
        /local/users/gauchard/arduino/gh-arduino-cli/arduino/builder/sketch.go:83
github.com/arduino/arduino-cli/arduino/builder.simpleLocalWalkRecursive
        /local/users/gauchard/arduino/gh-arduino-cli/arduino/builder/sketch.go:83
github.com/arduino/arduino-cli/arduino/builder.simpleLocalWalkRecursive
[...]

(depth detection error)

cmaglie · 2019-10-24T10:43:35Z

Maybe we should differentiate two cases here:

a symlink that has a cycle to himself -> easy
a symlink that points to a directory that contains another symlink that points back to (any parent of...) the first one -> hard

Here the tricky case (2):

:~/tmp$ mkdir A
:~/tmp$ mkdir B
:~/tmp$ cd A/
:~/tmp/A$ ln -s ../B/
:~/tmp/A$ cd ..
:~/tmp$ cd B
:~/tmp/B$ ln -s ../A/
:~/tmp/B$ cd ..
:~/tmp$ find -L .
.
./A
./A/B
find: File system loop detected; './A/B/A' is part of the same file system loop as './A'.
./B
./B/A
find: File system loop detected; './B/A/B' is part of the same file system loop as './B'.

it seems that ls has no way to list files following symlinks recursively (it follows symlinks only if they are listed on the command line as arguments).
find seems to have a good algorithm to detect loops, but it will require more work.

d-a-v · 2019-10-24T12:50:43Z

These two cases are handled by this PR.
Maybe I wasn't clear enough: the examples I show above are these two cases.
The tricky case is handled by the depth counter. The second quoted part of the examples is scrollable to the right and you can count the number of subdir levels: it's around 40.
Did I misunderstand something ?

About find -L's algorithm, I believe it retains every inode and stops with a symlink-loop error when the same inode is found again (golang FileInfo - where inode could be found I guess - is OS-dependent). We could do that but given the rarity of the issue, I believe a depth counter like implemented here is enough and easier to handle.

rsora · 2019-10-28T09:20:10Z

arduino/builder/sketch.go

+}
+
+// SimpleLocalWalk locally replaces filepath.Walk and/but goes through symlinks
+func SimpleLocalWalk(root string, walkFn func(path string, info os.FileInfo, err error) error) error {


The simpleLocalWalkRecursive() is currently used in this module only, There's no need to wrap it and export it. Can you please remove the wrapper and use directly simpleLocalWalkRecursive()?
Regarding the maxDepth param, can you please create a private constant in this module (named for example maxFileSystemDepth) documenting it properly writing something like: As currently implemented on Linux, the maximum number of symbolic links that will be followed while resolving a pathname is 40? This way you can use something more meaningful as a parameter when calling the function.
Thanks!

rsora · 2019-10-28T09:25:27Z

Hi @d-a-v! I left a comment on your code.
Can you please add a couple of tests to check the maxDepth control and the symlink loop?
You can see an example here on how to do it. The test code leverages sample sketches in the testdata folder, you can create a sketch that contains the symlinks you need for the testing

rsora · 2019-10-28T09:29:09Z

arduino/builder/sketch.go

+	err = SimpleLocalWalk(sketchFolder, func(path string, info os.FileInfo, err error) error {
+
+		if err != nil {
+			fmt.Printf("\nerror: %+v\n\n", err)


you can use the /cli/feedback package to print error messages like

feedback.Errorf("Error ....: %v", err)

d-a-v · 2019-10-28T18:55:28Z

@rsora I think I addressed your review.

masci · 2019-10-29T08:22:51Z

@d-a-v can you try manually running go fmt arduino/builder/sketch.go?

d-a-v · 2019-10-29T08:39:39Z

@masci
done, thanks

I don't understand CI messages though.

d-a-v · 2019-10-29T09:37:30Z

What about a script to run before pushing changes that would run CI tests locally ?

masci · 2019-10-29T09:41:53Z

@d-a-v our contributing guidelines already explain how to run tests locally before pushing code: https://github.com/arduino/arduino-cli/blob/master/CONTRIBUTING.md#running-the-tests

The style check, task check isn't mentioned though, I'll add it to the guide.

arduino/builder/sketch.go

rsora · 2019-10-29T09:46:59Z

Hi @d-a-v , left a comment regarding the error management.
Thanks for your time!

Co-Authored-By: Roberto Sora <[email protected]>

d-a-v · 2019-10-29T10:25:48Z

fixes addressed, CI passed, thanks for your feedback.

rsora

Excellent! 👍

Use a local Walk function

94e3d7b

Because arduino-cli segfaults when sketchDir is a symbolic link, a simple walk function is introduced with a twofold effect: - fix the segfault - allow to use symlinks ref: https://www.google.com/search?q=golang+Walk+does+not+follow+symbolic+links

d-a-v added 9 commits September 22, 2019 23:08

tabs

7186d73

style

23f0204

comment

fad3aa5

retrigger CI

7403a39

retrigger CI

9c82aad

Merge branch 'master' of github.com:arduino/arduino-cli

aac3d81

style

38c71e9

check sketch for being a file

5503c56

check sketch is not a directory

75ffd4f

d-a-v mentioned this pull request Sep 24, 2019

Building from local and modified sources ? arduino/arduino-builder#339

Closed

masci added the component/CLI label Oct 1, 2019

masci added this to the 0.6.0 milestone Oct 1, 2019

d-a-v mentioned this pull request Oct 1, 2019

Null pointer if target .ino is a folder #358

Closed

rsora modified the milestones: 0.6.0, 0.7.0 Oct 21, 2019

d-a-v added 3 commits October 23, 2019 21:56

Merge branch 'master' of github.com:arduino/arduino-cli

98bc2e3

additions:

05576c9

- act on error in walk function (OS can detect symbolic links loops (lnk -> lnk)) - add a deepness detector for non trivial loops (dir/lnk -> ../..)

fixes from "task check"

fe4cd8b

rsora reviewed Oct 28, 2019

View reviewed changes

updates per review

b51893a

style

cde4bec

rsora reviewed Oct 29, 2019

View reviewed changes

arduino/builder/sketch.go Outdated Show resolved Hide resolved

d-a-v and others added 2 commits October 29, 2019 10:50

Update arduino/builder/sketch.go

d41fa12

Co-Authored-By: Roberto Sora <[email protected]>

fixes

9de3344

rsora approved these changes Oct 29, 2019

View reviewed changes

rsora merged commit cf1c3ab into arduino:master Oct 29, 2019

rsora mentioned this pull request Oct 29, 2019

Add unit and integration testing for the sketch recursive load functionality #462

Merged

d-a-v mentioned this pull request Jan 4, 2021

ARDUIFINE: global compiler options extracted from ctags parsing #1117

Closed

4 tasks

silvanocerza mentioned this pull request Jul 9, 2021

[breaking] Refactor codebase to use a single Sketch struct #1353

Merged

5 tasks

per1234 mentioned this pull request Dec 16, 2021

Hang on symlink loop in library #1593

Closed

per1234 mentioned this pull request Mar 31, 2022

Cannot resolve library for nested files #484

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a local Walk function #421

Use a local Walk function #421

d-a-v commented Sep 22, 2019 •

edited

Loading

CLAassistant commented Sep 22, 2019 •

edited

Loading

cmaglie commented Oct 8, 2019

d-a-v commented Oct 9, 2019 •

edited

Loading

rsora commented Oct 15, 2019

d-a-v commented Oct 15, 2019

masci commented Oct 15, 2019

rsora commented Oct 15, 2019 •

edited

Loading

d-a-v commented Oct 23, 2019 •

edited

Loading

d-a-v commented Oct 24, 2019 •

edited

Loading

cmaglie commented Oct 24, 2019

d-a-v commented Oct 24, 2019 •

edited

Loading

rsora Oct 28, 2019

rsora commented Oct 28, 2019

rsora Oct 28, 2019

d-a-v commented Oct 28, 2019 •

edited

Loading

masci commented Oct 29, 2019

d-a-v commented Oct 29, 2019

d-a-v commented Oct 29, 2019

masci commented Oct 29, 2019

rsora commented Oct 29, 2019

d-a-v commented Oct 29, 2019

rsora left a comment

Use a local Walk function #421

Use a local Walk function #421

Conversation

d-a-v commented Sep 22, 2019 • edited Loading

CLAassistant commented Sep 22, 2019 • edited Loading

cmaglie commented Oct 8, 2019

d-a-v commented Oct 9, 2019 • edited Loading

rsora commented Oct 15, 2019

d-a-v commented Oct 15, 2019

masci commented Oct 15, 2019

rsora commented Oct 15, 2019 • edited Loading

d-a-v commented Oct 23, 2019 • edited Loading

d-a-v commented Oct 24, 2019 • edited Loading

cmaglie commented Oct 24, 2019

d-a-v commented Oct 24, 2019 • edited Loading

rsora Oct 28, 2019

Choose a reason for hiding this comment

rsora commented Oct 28, 2019

rsora Oct 28, 2019

Choose a reason for hiding this comment

d-a-v commented Oct 28, 2019 • edited Loading

masci commented Oct 29, 2019

d-a-v commented Oct 29, 2019

d-a-v commented Oct 29, 2019

masci commented Oct 29, 2019

rsora commented Oct 29, 2019

d-a-v commented Oct 29, 2019

rsora left a comment

Choose a reason for hiding this comment

d-a-v commented Sep 22, 2019 •

edited

Loading

CLAassistant commented Sep 22, 2019 •

edited

Loading

d-a-v commented Oct 9, 2019 •

edited

Loading

rsora commented Oct 15, 2019 •

edited

Loading

d-a-v commented Oct 23, 2019 •

edited

Loading

d-a-v commented Oct 24, 2019 •

edited

Loading

d-a-v commented Oct 24, 2019 •

edited

Loading

d-a-v commented Oct 28, 2019 •

edited

Loading